meta: allow vague objections to be dismissed #15233
Conversation
Explicitly allow vague objections to change requests to be dismissed if requests for clarification go unanswered
|
Maybe 48 hours as a time to respond when asked to clarify? |
|
@benjamingr ... I'd generally prefer not to give a specific time frame as what may be considered "reasonable" may vary depending on what the PR is changing |
|
I see this has a lot of approval, but I do wish to point out that we do already have a mechanism for overriding blocked PRs: Put it on the TSC agenda for a vote. If the TSC were getting a bunch of these every month and having to rubber-stamp PRs, then I could see the reason for wanting to add something like this. But this doesn't happen much AFAIK. This is not enough for me to object to this, but putting it out there for thought in case someone else thinks maybe it is. I'm kinda -0 on it. |
|
Also I fear this might get abused to dismiss objections that aren't vague. Maybe add an additional sentence and some typography to drive the point home:
|
|
@Trott ... The main goal of this is to reduce the need for frustratingly vague blocks on a pr from having to go through that more formal process and to encourage collaborators to be more cooperative in working through objections. |
| explanation or context*, and the objecting Collaborator fails to respond to | ||
| explicit requests for explanation or context within a reasonable period of | ||
| time, the objection may be dismissed. Note that this does not apply to | ||
| objections that are explained. |
There was a problem hiding this comment.
I actually went ahead and dismissed some reviews that were highly outdated after pinging the persons multiple times plus always asking them to do a new review. Most of them never replied at all and otherwise fine PRs stalled because of that even though it had lots of LGs. Those were mainly with explanation and context and the comments were addressed properly or it was clear after a discussion that something would not apply. So this is mainly not exactly the same but I fear that limiting this with the last sentence is a bad idea.
I know @Trott feared abuse but I personally think that anyone who is actually a collaborator should uphold a high standard and if that is not the case the other collaborators should get aware of that very fast and in that case measures must be taken anyway. Do we really have to limit us so badly?
There was a problem hiding this comment.
@BridgeAR AIUI this applies to changes that are not addressed because the reviewer wasn't clear about what they wanted.
The situations you're talking about sound like they're when the review has been addressed (either the change implemented, or a justification for not implementing provided). In that case if the reviewer doesn't respond that review can be considered addressed anyway, and shouldn't block landing.
There was a problem hiding this comment.
@BridgeAR I don't have a problem with the approach you describe. Although part of me does want to see those things get escalated to TSC because it might help surface inactive Collaborators who maybe shouldn't be Collaborators anymore. Our processes, coding conventions, and underlying philosophy all evolve over time. If someone is not really paying attention for two years or something, it's probably time to remove them. (If they want to get involved again, cool, but they should go through an onboarding.)
There was a problem hiding this comment.
@BridgeAR Maybe here or in a subsequent PR, you (or I or someone else) can offer up text to clarify that it's OK to dismiss another Collaborator's request if:
- it is believed in good faith that the request has been sufficiently addressed
AND
- attempts to engage the Collaborator in conversation about it have gone unanswered.
(There may already be text somewhere to that effect? I'm not sure.)
There was a problem hiding this comment.
I like that suggestion @Trott
I also see your point about escalating things to the TSC and we could maybe even have that in addition? So in case a review is dismissed because of one of the mentioned reasons, the TSC should also be notified about it? Even though it might at times also just be that someone is to involved and just does not handle all the notifications?
There was a problem hiding this comment.
Even though it might at times also just be that someone is to involved and just does not handle all the notifications?
Sure, but we'll know the difference between "this person is active in the repo but clearly just didn't get around to this issue for whatever reason" vs. "oh, yeah, haven't seen that person around for a year and a half".
Explicitly allow vague objections to change requests to be dismissed if requests for clarification go unanswered PR-URL: #15233 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Ryan Graham <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ron Korving <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
|
Landed in 2510500 |
Explicitly allow vague objections to change requests to be dismissed if requests for clarification go unanswered PR-URL: nodejs#15233 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Ryan Graham <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ron Korving <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Explicitly allow vague objections to change requests to be dismissed if requests for clarification go unanswered PR-URL: #15233 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Ryan Graham <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ron Korving <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
|
v6.x? |
|
ping @jasnell |
Explicitly allow vague objections to change requests to be dismissed if requests for clarification go unanswered PR-URL: #15233 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Ryan Graham <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ron Korving <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Explicitly allow vague objections to change requests to be dismissed if requests for clarification go unanswered
/cc @nodejs/collaborators